fix(memory): enforce authoritative revision integrity - #1303
Conversation
Teingi
left a comment
There was a problem hiding this comment.
Thanks for tightening the Memory integrity boundary. The original issue is real and the main reproducer is addressed, but the three cases below still allow revisions outside the documented contract. Requesting changes until these integrity gaps are closed.
| ) | ||
| ).mappings() | ||
| stored_rows = tuple(rows) | ||
| if any(str(row["entry_version_id"]) in new_by_id for row in stored_rows): |
There was a problem hiding this comment.
[P1] Make version-ID uniqueness database-enforced
Could we make this uniqueness check database-enforced? The SELECT and later INSERT are not atomic. On OceanBase, two transactions can both observe no row and insert the same entry_version_id for different memory_artifact_id values, because the current primary key and unique constraints include the Memory ID. Both commits can therefore succeed, leaving an ID that this code treats as scope-global duplicated. Please add a database-level UNIQUE (scope_id, entry_version_id) with an explicit migration for existing tables, translate the resulting integrity error, and cover it with a coordinated two-connection race where both reads complete before either insert.
There was a problem hiding this comment.
Thank you for catching this. I reproduced the two-transaction gap and confirmed that the SELECT/INSERT check was not sufficient. This is now fixed with a database-enforced unique index on (scope_id, entry_version_id), startup migrations for existing SQLite and OceanBase tables with duplicate-data detection, IntegrityError translation with rollback, and a coordinated opt-in OceanBase two-connection regression test.
| canonical_base: Memory | None, | ||
| new_by_entry: Mapping[str, MemoryEntryVersion], | ||
| ) -> None: | ||
| previous_ids = tuple( |
There was a problem hiding this comment.
[P1] Validate history already referenced by the base
This only collects predecessors for value.entry_versions, so an invalid version already referenced by canonical_base is never rechecked. I reproduced this by changing a persisted v1 row to previous_version_id="does-not-exist" and then appending an unrelated entry: the commit succeeds and the new Revision still references the broken history. Because older releases could already persist this state, please either validate the base-referenced chain here or provide a one-time audit/migration plus database constraints that make this assumption safe.
There was a problem hiding this comment.
Thank you for the detailed reproducer. I verified that an unrelated append could carry forward a broken predecessor already referenced by the canonical base. This is now fixed: the commit boundary validates every base-referenced chain back to v1, including identity, version sequence, predecessor links, revision ordering, and canonical hashes. A regression test covers corruption in a deeper ancestor.
| and change.from_entry_version_id == previous.entry_version_id | ||
| and change.to_entry_version_id == current.entry_version_id | ||
| and current.entry_version_id != previous.entry_version_id | ||
| and current.state == previous.state |
There was a problem hiding this comment.
[P2] Reject semantic revisions of inactive entries
This accepts a revise whenever the state is unchanged, including inactive -> inactive. A direct SPI writer can therefore deactivate an entry containing “black tea” and commit an inactive revision containing “green tea”, although the Memory contract requires reactivation before a semantic revision. organize(normalize) still needs a narrow exception, so please allow an inactive revision only when the new and predecessor canonical content bytes are identical and the change is normalization; otherwise require reactivation first.
There was a problem hiding this comment.
Thank you for pointing this out. I reproduced an inactive semantic revision and confirmed that it violated RFC 0014. This is now fixed: inactive revisions are rejected unless they are the narrow organize(normalize) case and the predecessor/new canonical content bytes are identical. Both the rejection and the allowed normalization path are covered by regression tests.
| except (TypeError, ValueError) as error: | ||
| raise _InvalidMemoryCommitError("vector") from error | ||
| if canonical != projection.embedding or expected_hash != projection.embedding_content_hash: | ||
| raise _InvalidMemoryCommitError("vector") |
There was a problem hiding this comment.
[P1] 不要用精确浮点相等判断单位向量已经规范化
MemoryService._embed_texts() 已先执行一次 canonical_embedding();这里再次归一化后再做 tuple 精确比较。单位归一化不是位级幂等的:公开 MemoryService.remember() 探针让模型返回 (0.2407121489724894, -0.9705965492093231),服务层得到 (0.24071214897248938, -0.9705965492093231),这里二次归一化又把首项改成 0.24071214897248935,于是合法的有限非零向量被抛为 _InvalidMemoryCommitError("vector"),整个 Memory 写入失败。现有向量测试只使用 (1, 0, 0) 这类轴向量,无法覆盖该舍入路径。请改为校验维度、有限性和带容差的单位范数(或确保只规范化一次并复用结果),并补一个非轴向量的公开 MemoryService.remember() 回归测试。
There was a problem hiding this comment.
感谢指正。我使用你给出的非轴向量复现并确认了这个问题:二次归一化后的精确浮点比较确实会误拒绝合法写入。现已修复为校验维度、有限性和带容差的单位范数,同时保留 profile/content hash 校验,并补充了公开 MemoryService.remember() 稠密向量回归测试及非单位向量拒绝测试。
Which issue or RFC does this PR close?
Closes #1297.
Rationale for this change
MemoryUnitOfWork.commit()is the final persistence trust boundary, but it previously accepted incomplete or internally inconsistent revisions. A malformed direct SPI writer could persist illegal predecessor links or body/hash mismatches, and search could then return text that authoritative entry reads rejected.What changes are included in this PR?
entry_version_ididentity with a database unique index, including startup migration and duplicate-data detection for existing SQLite and OceanBase schemas; translate insert races intoMemoryBackendConfigurationErrorwith full rollback.organize(normalize)path when canonical content bytes are unchanged.entries(),expand(), projection reads, and every FTS/vector candidate against the exact authoritative revision, manifest, entry row, and current head projection.Are there any user-facing changes?
Malformed commits made through the public Memory backend SPI now fail with
MemoryBackendConfigurationErrorinstead of becoming durable state. Existing SQLite and OceanBase schemas receive the scope-global entry-version unique index during startup; startup reportsMemoryBackendConfigurationErrorif legacy duplicate identities must be repaired first. Valid service workflows and the documented no-vector degradation remain unchanged. There are no HTTP API changes.How was this change tested?
uv lock --lockeduv run --no-sync prek run -auv run --no-sync ty checkuv run --no-sync python -m pytest tests/builtin/artifacts/memory tests/builtin/persistence/test_memory.py tests/builtin/persistence/test_memory_integrity.py tests/builtin/persistence/test_sqlite_profile.py tests/builtin/persistence/test_oceanbase_profile.py tests/e2e/test_memory_search_concurrency.py tests/e2e/test_builtin_runtime.py -q(60 passed, 7 skipped)620 passed, 15 skipped; 8 unrelated pre-existing Windows failures remain in symlink privileges, the unavailablepython3command, POSIX mode bits, encoding, and path-separator assertions.POWERCONTEXT_TEST_OCEANBASE_URL; it is skipped when no dedicated OceanBase test database is configured.AI usage statement
OpenAI Codex (GPT-5) was used to inspect the issue and review feedback, implement the fixes, design regression tests, and review the final change.